Implement scantxoutset method and test #164

GideonBature · 2025-05-12T23:20:53Z

The JSON-RPC method scantxoutset does return a type. We want to test this to catch any changes in behavior in future Core versions.

This PR adds a client function that errors if the return value is anything other than the type it returns, along with an integration test that calls this function.

Ref: #116
Closes #115

tcharding · 2025-05-13T04:23:56Z

Why is there identical code in the v17 and v18 blockchain modules of the client?

GideonBature · 2025-05-13T04:50:25Z

Why is there identical code in the v17 and v18 blockchain modules of the client?

They basically work the same way, the difference is in their return type, v18 has an additional field (desc) in it's result, whereas it is lacking in v17. Or is there a way to reuse the client code in v17 for v18 and still be able to return the type that is meant for v18?

tcharding · 2025-05-13T22:44:21Z

Bro I explained this to you already on the other two PRs you did. Perhaps go back over them and see if you can work it out.

GideonBature · 2025-05-14T17:09:45Z

Bro I explained this to you already on the other two PRs you did. Perhaps go back over them and see if you can work it out.

oh, I now understand, I was looking at how gettxout was used in the client, where despite the code is similar, they used it for v17 and again for v22. So I thought that for any change in the fields of return type of a method, there should also be a code in the module of the client for that particular version with the change. But I can see that it's not always the case.

For this recent push, I removed the repeated code across all versions having different fields in their type.

tcharding · 2025-05-15T00:04:59Z

Oh well that would explain your confusion, the gettxout in v22 is just plain wrong. I'll remove it.

tcharding · 2025-05-15T00:09:24Z

Done in #169. FTR bugs happen, if you see things that are wrong or look weird just ask (or PR to fix them). Apologies for being so fierce yesterday.

tcharding · 2025-05-15T02:19:12Z

Returns column needs updating (and Notes).

tcharding

Holy moly you picked a difficult one. I'm going to have to come back to this when I can put more thought into it. Feel free to ignore my suggestions until then.

integration_test/tests/blockchain.rs

types/src/v17/blockchain/mod.rs

GideonBature · 2025-05-15T14:01:16Z

Done in #169. FTR bugs happen, if you see things that are wrong or look weird just ask (or PR to fix them). Apologies for being so fierce yesterday.

No problem. I will do this going forward.

GideonBature · 2025-05-15T20:48:59Z

Returns column needs updating (and Notes).

Noted!

GideonBature · 2025-05-15T20:49:19Z

Holy moly you picked a difficult one. I'm going to have to come back to this when I can put more thought into it. Feel free to ignore my suggestions until then.

Noted....

GideonBature · 2025-05-15T21:09:59Z

Returns column needs updating (and Notes).

for the notes, is there any need to change it? since it describes perfectly that the scantxoutset is experimental.

tcharding · 2025-05-16T00:19:53Z

Oh you are right, lets leave the notes as they are.

tcharding · 2025-05-27T05:41:20Z

Man this method is really not that simple to add support for because it changes so often. Can we put this one on hold until you've got a few more easy ones done?

GideonBature · 2025-05-28T22:33:43Z

Man this method is really not that simple to add support for because it changes so often. Can we put this one on hold until you've got a few more easy ones done?

Understood... sure, let's do that.

GideonBature · 2025-07-15T14:49:49Z

Complete the scantxoutset method and model.

types/src/model/blockchain.rs

tcharding · 2025-07-15T23:23:40Z

types/src/model/blockchain.rs

+}
+
+#[derive(Deserialize, Debug, Clone, PartialEq)]
+pub struct ScanTxOutSetStatus {


Observation: This struct never changes

Use git grep -A 4 'pub struct ScanTxOutSetStatus' to see.

tcharding · 2025-07-15T23:28:59Z

types/src/v18/blockchain/mod.rs

+}
+
+#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
+pub struct ScanTxOutSetStart {


This looks wrong. Perhaps add #[serde(deny_unknown_fields)] to all your structs and that will help find the bugs.

I added #[serde(deny_unknown_fields)] to this to test, and there are two missing fields, success and searched_items.

tcharding

You've done a bunch of work, well done. This one is pretty curly, here are some ideas.

I think it would be more simple if we were to split up the return types for the three different commands (start, abort, status). That way we only have to re-implement the ones that change.

In the client I think it would be more simple to have three methods, one for each command and not have a ScanAction type.

Instead of ScanObject I think we can just use a String and ignore the added complexity of an object (the WithDesc variant) since it is unused. In general we don't want to add code to the client that we do not use in testing.

That should give you a bit to work on!

jamillambert

There are some missing renames, relates to issue #298. I haven't necessarily highlighted all of them.

types/src/v19/blockchain/mod.rs

types/src/v25/blockchain/mod.rs

types/src/v28/blockchain/mod.rs

GideonBature · 2025-07-19T16:47:58Z

You've done a bunch of work, well done. This one is pretty curly, here are some ideas.

I think it would be more simple if we were to split up the return types for the three different commands (start, abort, status). That way we only have to re-implement the ones that change.

In the client I think it would be more simple to have three methods, one for each command and not have a ScanAction type.

Instead of ScanObject I think we can just use a String and ignore the added complexity of an object (the WithDesc variant) since it is unused. In general we don't want to add code to the client that we do not use in testing.

That should give you a bit to work on!

This suggestion is much better. Will work with it. Thank you.

Split single return type to three types Rename some fields for readability Rename some fields for readability

GideonBature · 2025-07-19T17:19:51Z

Implemented suggested changes.

Format code

jamillambert

I made a few comments, they apply to all affected versions.

jamillambert · 2025-07-21T19:14:22Z

client/src/client_sync/v29/mod.rs

@@ -19,7 +19,8 @@ use crate::types::v29::*;

 #[rustfmt::skip]                // Keep public re-exports separate.
 pub use crate::client_sync::{
-    v17::{AddNodeCommand, ImportMultiRequest, ImportMultiScriptPubKey, ImportMultiTimestamp, Input, Output, SetBanCommand, WalletCreateFundedPsbtInput,},
+    v17::{AddNodeCommand, ImportMultiRequest, ImportMultiScriptPubKey, ImportMultiTimestamp, Input, Output, SetBanCommand, WalletCreateFundedPsbtInput


nit: This change is unrelated and wrong. It's not fixed by cargo fmt due to the skip above the block.

NB. I'm ok with the random fix being in this PR if done correctly though.

jamillambert · 2025-07-21T19:34:15Z

types/src/v17/blockchain/mod.rs

+
+#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
+pub struct ScanTxOutSetAbort(pub bool);


Needs rustdocs

jamillambert · 2025-07-21T19:36:01Z

types/src/v17/blockchain/mod.rs

+#[derive(Deserialize, Debug, Clone, PartialEq)]
+pub struct ScanTxOutSetStatus {
+    /// Approximate percent complete
+    pub progress: f64,
+}


Suggested change

#[derive(Deserialize, Debug, Clone, PartialEq)]

pub struct ScanTxOutSetStatus {

/// Approximate percent complete

pub progress: f64,

}

#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]

pub struct ScanTxOutSetStatus(pub f64);

Also needs docs. Was there a reason serialize was not included that I have missed?

jamillambert · 2025-07-21T19:41:05Z

integration_test/tests/blockchain.rs

+
+    let _: Option<ScanTxOutSetStatus> = node.client.scan_tx_out_set_status().expect("scantxoutset status");
+
+    let model: Result<mtype::ScanTxOutSetStart, ScanTxOutSetError> = json.into_model();


Moving this line to directly below let json makes more sense. I had to look twice and first thought the model for ..Status was being checked and not ..Start.

jamillambert · 2025-07-21T19:43:27Z

types/src/model/blockchain.rs

+    pub total_amount: Amount,
+}
+
+#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]


Suggested change

#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]

/// Unspents item returned as part of `scantxoutset`

#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]

Needs docs, e.g. above. Having scanned through the code a lot recently I find it useful if the original RPC is mentioned.

jamillambert · 2025-07-21T21:03:09Z

types/src/v18/blockchain/mod.rs

+}
+
+#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
+pub struct ScanTxOutSetStart {


I added #[serde(deny_unknown_fields)] to this to test, and there are two missing fields, success and searched_items.

jamillambert · 2025-07-21T21:05:49Z

types/src/v25/blockchain/mod.rs

+/// > Returns an object containing various state info regarding blockchain processing.
+#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct GetBlockchainInfo {


Most of the changes to this file are not part of this PR, left over from a copy and paste from v19?

GideonBature force-pushed the scantxoutset branch 3 times, most recently from 63e4f61 to 67b03b6 Compare May 12, 2025 23:49

GideonBature force-pushed the scantxoutset branch from 67b03b6 to 43c8b61 Compare May 14, 2025 17:33

tcharding reviewed May 15, 2025

View reviewed changes

integration_test/tests/blockchain.rs Outdated Show resolved Hide resolved

types/src/v17/blockchain/mod.rs Outdated Show resolved Hide resolved

types/src/v17/blockchain/mod.rs Outdated Show resolved Hide resolved

GideonBature force-pushed the scantxoutset branch from 43c8b61 to 9b7f019 Compare May 15, 2025 20:58

GideonBature force-pushed the scantxoutset branch from 9b7f019 to 1a3bd20 Compare May 15, 2025 21:10

GideonBature force-pushed the scantxoutset branch from 1a3bd20 to 901bf21 Compare May 18, 2025 13:34

tcharding marked this pull request as draft May 27, 2025 05:41

GideonBature closed this May 31, 2025

GideonBature deleted the scantxoutset branch May 31, 2025 08:09

GideonBature restored the scantxoutset branch May 31, 2025 08:35

GideonBature reopened this May 31, 2025

github-actions bot added C-client C-integration-test C-types labels May 31, 2025

GideonBature force-pushed the scantxoutset branch from 901bf21 to 1bcfa84 Compare July 15, 2025 14:41

GideonBature marked this pull request as ready for review July 15, 2025 14:42

GideonBature requested a review from tcharding July 15, 2025 14:50

tcharding reviewed Jul 15, 2025

View reviewed changes

types/src/model/blockchain.rs Outdated Show resolved Hide resolved

tcharding reviewed Jul 15, 2025

View reviewed changes

jamillambert reviewed Jul 18, 2025

View reviewed changes

GideonBature force-pushed the scantxoutset branch from 4c0d8c7 to 22d3d22 Compare July 19, 2025 16:42

GideonBature force-pushed the scantxoutset branch from 7d0db23 to 9fa612d Compare July 19, 2025 17:04

Implement scantxoutset method and test

de771f3

Split single return type to three types Rename some fields for readability Rename some fields for readability

GideonBature force-pushed the scantxoutset branch from 936dc46 to 678a371 Compare July 19, 2025 17:18

Format code

52d9d1c

Format code

GideonBature force-pushed the scantxoutset branch from 678a371 to 52d9d1c Compare July 19, 2025 17:22

GideonBature requested review from tcharding and jamillambert July 21, 2025 19:49

jamillambert reviewed Jul 21, 2025

View reviewed changes


		#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
		pub struct ScanTxOutSetAbort(pub bool);


		let _: Option<ScanTxOutSetStatus> = node.client.scan_tx_out_set_status().expect("scantxoutset status");

		let model: Result<mtype::ScanTxOutSetStart, ScanTxOutSetError> = json.into_model();

	#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
	/// Unspents item returned as part of `scantxoutset`
	#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]

Implement scantxoutset method and test #164

Are you sure you want to change the base?

Implement scantxoutset method and test #164

Uh oh!

Conversation

GideonBature commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented May 13, 2025

Uh oh!

GideonBature commented May 13, 2025

Uh oh!

tcharding commented May 13, 2025

Uh oh!

GideonBature commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented May 15, 2025

Uh oh!

tcharding commented May 15, 2025

Uh oh!

tcharding commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GideonBature commented May 15, 2025

Uh oh!

GideonBature commented May 15, 2025

Uh oh!

GideonBature commented May 15, 2025

Uh oh!

GideonBature commented May 15, 2025

Uh oh!

tcharding commented May 16, 2025

Uh oh!

tcharding commented May 27, 2025

Uh oh!

GideonBature commented May 28, 2025

Uh oh!

GideonBature commented Jul 15, 2025

Uh oh!

Uh oh!

tcharding Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

tcharding Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

jamillambert Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

jamillambert left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GideonBature commented Jul 19, 2025

Uh oh!

GideonBature commented Jul 19, 2025

Uh oh!

jamillambert left a comment

Choose a reason for hiding this comment

Uh oh!

jamillambert Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

GideonBature commented May 12, 2025 •

edited

Loading

GideonBature commented May 14, 2025 •

edited

Loading

tcharding commented May 15, 2025 •

edited

Loading

jamillambert Jul 21, 2025 •

edited

Loading

jamillambert left a comment •

edited

Loading

jamillambert Jul 21, 2025 •

edited

Loading

jamillambert Jul 21, 2025 •

edited

Loading

jamillambert Jul 21, 2025 •

edited

Loading